Skip to content

Sanitizers target modificators #138736

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

azhogin
Copy link
Contributor

@azhogin azhogin commented Mar 20, 2025

Depends on bool flag fix: #138483.

Some sanitizers need to be target modifiers, and some do not. For now, we should mark all sanitizers as target modifiers except for these: AddressSanitizer, LeakSanitizer

For kCFI, the helper flag -Zsanitizer-cfi-normalize-integers should also be a target modifier.

Many test errors was with sanizer flags inconsistent with std deps. Tests are fixed with -C unsafe-allow-abi-mismatch.

@rustbot
Copy link
Collaborator

rustbot commented Mar 20, 2025

r? @BoxyUwU

rustbot has assigned @BoxyUwU.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 20, 2025
@rust-log-analyzer

This comment has been minimized.

@azhogin azhogin force-pushed the azhogin/sanitizers-target-modificators branch from 7526d38 to 0777169 Compare March 20, 2025 12:59
@rust-log-analyzer

This comment has been minimized.

@azhogin azhogin force-pushed the azhogin/sanitizers-target-modificators branch from 0777169 to f454b02 Compare March 24, 2025 08:53
@rustbot rustbot added the PG-exploit-mitigations Project group: Exploit mitigations label Mar 24, 2025
@rust-log-analyzer

This comment has been minimized.

@azhogin azhogin force-pushed the azhogin/sanitizers-target-modificators branch from f454b02 to 509e0b3 Compare March 24, 2025 09:37
@rust-log-analyzer

This comment has been minimized.

@azhogin azhogin force-pushed the azhogin/sanitizers-target-modificators branch from 509e0b3 to 1f20a51 Compare March 24, 2025 11:33
@rust-log-analyzer

This comment has been minimized.

@azhogin azhogin force-pushed the azhogin/sanitizers-target-modificators branch from 1f20a51 to fc95e77 Compare March 24, 2025 13:14
@rust-log-analyzer

This comment has been minimized.

@azhogin azhogin force-pushed the azhogin/sanitizers-target-modificators branch 2 times, most recently from e025a55 to c54dcfc Compare March 24, 2025 13:40
@azhogin azhogin marked this pull request as ready for review March 24, 2025 14:46
@rustbot
Copy link
Collaborator

rustbot commented Mar 24, 2025

Some changes occurred in tests/codegen/sanitizer

cc @rust-lang/project-exploit-mitigations, @rcvalle

Some changes occurred in tests/ui/sanitizer

cc @rust-lang/project-exploit-mitigations, @rcvalle

@bors
Copy link
Collaborator

bors commented Mar 26, 2025

☔ The latest upstream changes (presumably #138974) made this pull request unmergeable. Please resolve the merge conflicts.

@BoxyUwU
Copy link
Member

BoxyUwU commented Mar 26, 2025

r? compiler

@Darksonn
Copy link
Contributor

Do we have any tests that check simultaneous use of multiple sanitizers? For example, if I enable both shadow-call-stack and kcfi at the same time, then both must also be enabled in deps.

@rcvalle rcvalle added the A-sanitizers Area: Sanitizers for correctness and code quality label Apr 10, 2025
@rcvalle
Copy link
Member

rcvalle commented Apr 10, 2025

@rustbot claim

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 10, 2025
@bors
Copy link
Collaborator

bors commented Jun 13, 2025

☔ The latest upstream changes (presumably #142443) made this pull request unmergeable. Please resolve the merge conflicts.

@Darksonn
Copy link
Contributor

@azhogin Any chance you could look at this again?

@azhogin
Copy link
Contributor Author

azhogin commented Aug 11, 2025

@azhogin Any chance you could look at this again?

Sure, I will.

@azhogin azhogin force-pushed the azhogin/sanitizers-target-modificators branch from f04eb8c to 455b1c6 Compare August 12, 2025 12:33
@rustbot
Copy link
Collaborator

rustbot commented Aug 12, 2025

Some changes occurred in tests/codegen-llvm/sanitizer

cc @rcvalle

@rust-log-analyzer

This comment has been minimized.

@azhogin azhogin force-pushed the azhogin/sanitizers-target-modificators branch from 455b1c6 to 2106c51 Compare August 12, 2025 16:38
@rust-log-analyzer

This comment has been minimized.

@azhogin azhogin force-pushed the azhogin/sanitizers-target-modificators branch from 2106c51 to 5551d8c Compare August 12, 2025 18:39
@rcvalle
Copy link
Member

rcvalle commented Aug 12, 2025

@bors try

rust-bors bot added a commit that referenced this pull request Aug 12, 2025
…s, r=<try>

Sanitizers target modificators
@rust-bors

This comment has been minimized.

@rust-bors
Copy link

rust-bors bot commented Aug 12, 2025

☀️ Try build successful (CI)
Build commit: c76ae33 (c76ae331921751a325be568fb8cb1f8630735711, parent: 8e62bfd311791bfd9dca886abdfbab07ec54d8b4)

@rcvalle
Copy link
Member

rcvalle commented Aug 13, 2025

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 13, 2025

📌 Commit 5551d8c has been approved by rcvalle

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 13, 2025
Zalathar added a commit to Zalathar/rust that referenced this pull request Aug 13, 2025
…modificators, r=rcvalle

Sanitizers target modificators

Depends on bool flag fix: rust-lang#138483.

Some sanitizers need to be target modifiers, and some do not. For now, we should mark all sanitizers as target modifiers except for these: AddressSanitizer, LeakSanitizer

For kCFI, the helper flag -Zsanitizer-cfi-normalize-integers should also be a target modifier.

Many test errors was with sanizer flags inconsistent with std deps. Tests are fixed with `-C unsafe-allow-abi-mismatch`.
jdonszelmann added a commit to jdonszelmann/rust that referenced this pull request Aug 13, 2025
…modificators, r=rcvalle

Sanitizers target modificators

Depends on bool flag fix: rust-lang#138483.

Some sanitizers need to be target modifiers, and some do not. For now, we should mark all sanitizers as target modifiers except for these: AddressSanitizer, LeakSanitizer

For kCFI, the helper flag -Zsanitizer-cfi-normalize-integers should also be a target modifier.

Many test errors was with sanizer flags inconsistent with std deps. Tests are fixed with `-C unsafe-allow-abi-mismatch`.
bors added a commit that referenced this pull request Aug 13, 2025
Rollup of 9 pull requests

Successful merges:

 - #122661 (Change the desugaring of `assert!` for better error output)
 - #138736 (Sanitizers target modificators)
 - #144955 (search graph: lazily update parent goals)
 - #145120 (llvm: Accept new LLVM lifetime format)
 - #145153 (Handle macros with multiple kinds, and improve errors)
 - #145189 (Weekly `cargo update`)
 - #145250 (Add regression test for former ICE involving malformed meta items containing interpolated tokens)
 - #145309 (Fix `-Zregparm` for LLVM builtins)
 - #145331 (Make std use the edition 2024 prelude)

r? `@ghost`
`@rustbot` modify labels: rollup
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test failed on aarch64-apple

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@azhogin I know it's been difficult to merge this. Sorry about that. Would you mind taking a look at it again whenever you have time?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am looking at this atm, just don't understand the problem with tests/ui/sanitizer/address.rs.
Did I understand it correctly, that with new changes, the test doesn't generate expected sanitizer error? Or it just generates at different line?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error is generated at a different line, i.e., line 15.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I have updated tests to minimize line offsets changes that may cause problems in different architectures.

@fmease
Copy link
Member

fmease commented Aug 13, 2025

#145351 (comment)
@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 13, 2025
…t modifiers with custom consistency check function
@azhogin azhogin force-pushed the azhogin/sanitizers-target-modificators branch from 5551d8c to bf38919 Compare August 13, 2025 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-run-make Area: port run-make Makefiles to rmake.rs A-sanitizers Area: Sanitizers for correctness and code quality PG-exploit-mitigations Project group: Exploit mitigations S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output.
Projects
None yet
Development

Successfully merging this pull request may close these issues.